src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (2)

13-19: Events fetch still lacks error handling and can mask pod failures.

This is a duplicate of a previous review comment. If listNamespacedEvent fails due to RBAC restrictions, API timeouts, or other errors, the exception will propagate and obscure the actual pod failure. Since events are diagnostic-only, a failure here shouldn't prevent reporting the pod's failed state.

Apply the suggested fix from the previous review:

-      const events = (await kubeClient.listNamespacedEvent(namespace)).body.items
-        .filter((x) => x.involvedObject?.name === podName)
-        .map((x) => ({
-          message: x.message || '',
-          reason: x.reason || '',
-          type: x.type || '',
-        }));
+      let events: Array<{ message: string; reason: string; type: string }> = [];
+      try {
+        events = (await kubeClient.listNamespacedEvent(namespace)).body.items
+          .filter((x) => x.involvedObject?.name === podName)
+          .map((x) => ({
+            message: x.message || '',
+            reason: x.reason || '',
+            type: x.type || '',
+          }));
+      } catch (eventError) {
+        CloudRunnerLogger.logWarning(`Failed to fetch pod events: ${eventError}`);
+      }

---

35-58: Multi-container logic flaw persists: only the last container's exit status is retained.

This is a duplicate of a previous review comment. The loop iterates over all container statuses but overwrites containerExitCode and containerSucceeded on each iteration. If a pod has multiple containers and the last one succeeded but an earlier one failed, the code will incorrectly treat the pod as successful and skip error throwing at line 98. This critical flaw also affects the PreStopHook handling logic at lines 64-84, which relies on accurate container exit status.

Apply the suggested fix from the previous review to check all containers:

-      let containerExitCode: number | undefined;
-      let containerSucceeded = false;
+      let allContainersSucceeded = true;
+      let anyContainerExitCode: number | undefined;
       
       if (containerStatuses.length > 0) {
         containerStatuses.forEach((cs, idx) => {
           if (cs.state?.waiting) {
             errorDetails.push(
               `Container ${idx} (${cs.name}) waiting: ${cs.state.waiting.reason} - ${cs.state.waiting.message || ''}`,
             );
           }
           if (cs.state?.terminated) {
             const exitCode = cs.state.terminated.exitCode;
-            containerExitCode = exitCode;
+            anyContainerExitCode = exitCode;
             if (exitCode === 0) {
-              containerSucceeded = true;
+              // Keep checking all containers
+            } else {
+              allContainersSucceeded = false;
             }
             errorDetails.push(
               `Container ${idx} (${cs.name}) terminated: ${cs.state.terminated.reason} - ${
                 cs.state.terminated.message || ''
               } (exit code: ${exitCode})`,
             );
           }
         });
       }

Then update lines 70 and 88 to use allContainersSucceeded and anyContainerExitCode.

src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (1)

86-94: Clarify comment about error throwing behavior.

The comment at lines 92-93 states "Still throw error for now" but there's no explicit throw statement in this block. The code falls through to line 98 where the error is actually thrown. Consider updating the comment to be more explicit about the fall-through behavior.

-        // Still throw error for now, but with more context
-        // The task runner will retry and get the actual container status
+        // Fall through to throw error with enhanced context
+        // The task runner will retry and should get the actual container status

src/model/cloud-runner/workflows/custom-workflow.ts (1)

46-62: Use CloudRunnerLogger.error for build-stopping failures.

At lines 58-60, when allowFailure is false and the build stops, use error instead of log for proper severity signaling in CI logs. This matches the warning level used when the step fails but continues.

         if (allowFailure) {
           CloudRunnerLogger.logWarning(
             `Hook container "${stepName}" failed but allowFailure is true. Continuing build. Error: ${
               error?.message || error
             }`,
           );
           // Continue to next step
         } else {
-          CloudRunnerLogger.log(
+          CloudRunnerLogger.error(
             `Hook container "${stepName}" failed and allowFailure is false (default). Stopping build.`,
           );
           throw error;
         }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.

src/model/cloud-runner/remote-client/caching.ts (3)

121-126: Command injection concern remains unaddressed.

The shell command at line 123 interpolates cacheArtifactName and compressionSuffix directly. This was previously flagged - the sanitization at line 48 (removing spaces) is insufficient to prevent command injection via shell metacharacters.

---

132-136: String-based error detection remains fragile.

The error detection at line 135 checking for 'No space left' or 'Wrote only' was previously flagged. Different systems, tar implementations, or locales may produce different error messages. Consider also checking for ENOSPC or the error code:

const isSpaceError = errorMessage.includes('No space left') || 
                     errorMessage.includes('Wrote only') ||
                     errorMessage.includes('ENOSPC') ||
                     error?.code === 'ENOSPC';

---

101-111: Cleanup may target the working directory when cacheFolder is a relative path.

When cacheFolder is 'cache' (the default per CloudRunnerFolders.cacheFolder), path.dirname(cacheFolder) returns '.'. This causes the find commands at lines 106 and 110 to scan the current working directory, potentially deleting unrelated *.tar* files or directories.

         try {
           const cacheParent = path.dirname(cacheFolder);
-          if (await fileExists(cacheParent)) {
+          // Only clean up if cacheFolder is an absolute path with a distinct parent
+          if (cacheParent !== '.' && cacheParent !== cacheFolder && await fileExists(cacheParent)) {

Alternatively, restrict cleanup to the cache folder itself rather than its parent.

src/model/cloud-runner/remote-client/caching.ts (1)

85-91: Fragile disk usage parsing.

The regex /(\d+)%/ captures the first percentage in df output, but df output format varies across systems and locales. Some systems show capacity percentage in different columns, and the mount point might also contain numbers followed by % in edge cases.

Consider parsing specific columns or using df --output=pcent where available:

-        const diskCheckOutput = await CloudRunnerSystem.Run(`df . 2>/dev/null || df /data 2>/dev/null || true`);
+        const diskCheckOutput = await CloudRunnerSystem.Run(`df --output=pcent . 2>/dev/null | tail -1 || df . 2>/dev/null | awk 'NR==2 {print $5}' || true`);
         CloudRunnerLogger.log(`Disk space before tar: ${diskCheckOutput}`);
-        // Parse disk usage percentage (e.g., "72G  72G  196M 100%")
-        const usageMatch = diskCheckOutput.match(/(\d+)%/);
+        const usageMatch = diskCheckOutput.trim().match(/(\d+)%?$/);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (2)

13-19: Wrap events fetch in try-catch to prevent masking pod failures.

If listNamespacedEvent fails (e.g., RBAC restrictions, API timeout), the exception will propagate and obscure the actual pod failure. Since events are diagnostic-only, a failure here shouldn't prevent reporting the pod's failed state.

This issue was previously identified in past review comments. Consider implementing the suggested fix:

-      const events = (await kubeClient.listNamespacedEvent(namespace)).body.items
-        .filter((x) => x.involvedObject?.name === podName)
-        .map((x) => ({
-          message: x.message || '',
-          reason: x.reason || '',
-          type: x.type || '',
-        }));
+      let events: Array<{ message: string; reason: string; type: string }> = [];
+      try {
+        events = (await kubeClient.listNamespacedEvent(namespace)).body.items
+          .filter((x) => x.involvedObject?.name === podName)
+          .map((x) => ({
+            message: x.message || '',
+            reason: x.reason || '',
+            type: x.type || '',
+          }));
+      } catch (eventError) {
+        CloudRunnerLogger.logWarning(`Failed to fetch pod events: ${eventError}`);
+      }

---

35-84: Multi-container logic flaw: only the last container's exit status is retained.

The loop at lines 39-57 overwrites containerExitCode and containerSucceeded on each iteration. Consequently, the check at line 70 (containerSucceeded && containerExitCode === 0) only verifies whether the last container succeeded, ignoring failures in earlier containers.

Scenario: If a pod has multiple containers where the last one exits with code 0 but an earlier one failed, the code will incorrectly return false at line 83, treating the pod as successful and bypassing the error throw at line 130.

This issue was previously identified in past review comments. Consider the suggested fix to check all containers:

-      let containerExitCode: number | undefined;
-      let containerSucceeded = false;
+      let allContainersSucceeded = true;
+      let anyContainerExitCode: number | undefined;
       
       if (containerStatuses.length > 0) {
         containerStatuses.forEach((cs, idx) => {
           if (cs.state?.waiting) {
             errorDetails.push(
               `Container ${idx} (${cs.name}) waiting: ${cs.state.waiting.reason} - ${cs.state.waiting.message || ''}`,
             );
           }
           if (cs.state?.terminated) {
             const exitCode = cs.state.terminated.exitCode;
-            containerExitCode = exitCode;
+            anyContainerExitCode = exitCode;
             if (exitCode === 0) {
-              containerSucceeded = true;
+              // Keep checking all containers
+            } else {
+              allContainersSucceeded = false;
             }
             errorDetails.push(
               `Container ${idx} (${cs.name}) terminated: ${cs.state.terminated.reason} - ${
                 cs.state.terminated.message || ''
               } (exit code: ${exitCode})`,
             );
           }
         });
       }

Then update line 70 to: if (allContainersSucceeded && anyContainerExitCode === 0)

src/model/cloud-runner/remote-client/caching.ts (4)

153-171: retrySucceeded flag is unreachable noise; simplify the retry block.

If the retry tar command throws, the inner catch already throws a new error, so execution never reaches the if (!retrySucceeded) check. If it succeeds, retrySucceeded is true, so the condition is false. The flag and subsequent if are therefore dead code.

You can simplify to just a try/catch:

-              // Retry the tar operation once after cleanup
-              let retrySucceeded = false;
               try {
                 await CloudRunnerSystem.Run(
                   `tar -cf ${cacheArtifactName}.tar${compressionSuffix} "${path.basename(sourceFolder)}"`,
                 );
-                // If retry succeeds, mark it - we'll continue normally without throwing
-                retrySucceeded = true;
               } catch (retryError: any) {
                 throw new Error(
                   `Failed to create cache archive after cleanup. Original error: ${errorMessage}. Retry error: ${
                     retryError?.message || retryError
                   }`,
                 );
               }
-              // If retry succeeded, don't throw the original error - let execution continue after catch block
-              if (!retrySucceeded) {
-                throw error;
-              }
-              // If we get here, retry succeeded - execution will continue after the catch block

---

96-107: Restrict cache cleanup find scope to avoid deleting unrelated .tar files.

Using cacheParent = path.dirname(cacheFolder) (and later cacheRoot = path.resolve(cacheParent, '..')) means the cleanup can target . or even /, and the pattern -name "*.tar*" will match any .tar-like file under those trees. On shared runners this risks deleting non-cache archives or other .tar data.

Consider tightening this by:

• Limiting cleanup to a well-defined cache root (e.g., cacheFolder or a dedicated cache base), not its parent or grandparent.
• Using a stricter filename pattern (e.g., known cache prefixes / suffixes) instead of "*.tar*" so only your cache archives are affected.
• Optionally making the TTLs (360/60/30 minutes) configurable rather than hard-coded.

Also applies to: 135-150

---

104-107: Shell commands interpolate unvalidated paths/artifact names, leaving command‑injection risk.

cacheArtifactName, cacheFolder, cacheParent, and cacheRoot flow from CLI/env and are interpolated directly into shell commands (rm, tar, find, mv) without robust validation. The .replace(' ', '') on cacheArtifactName does not prevent shell metacharacters like ;, &, |, $, or backticks. A crafted artifact name or cache path could break out of the intended command and execute arbitrary shell.

To harden this, either:

• Prefer Node APIs (fs.promises.rm, fs.promises.readdir/stat, a tar library, etc.) with path.join/path.resolve instead of shell rm/find/tar/mv, or
• Introduce strict validation/quoting for any value interpolated into shell strings (e.g., whitelisting [A-Za-z0-9._/-] and rejecting anything else, or wrapping via a dedicated quoting helper).

For example, a minimal mitigation pattern:

+const safeShellArg = (value: string): string => {
+  if (!/^[A-Za-z0-9._\/-]+$/.test(value)) {
+    throw new Error(`Invalid shell argument: ${value}`);
+  }
+  return value;
+};
...
-        await CloudRunnerSystem.Run(`rm -f ${cacheArtifactName}.tar${compressionSuffix} 2>/dev/null || true`);
+        await CloudRunnerSystem.Run(
+          `rm -f ${safeShellArg(cacheArtifactName)}.tar${compressionSuffix} 2>/dev/null || true`,
+        );
...
-              `find ${cacheParent} -name "*.tar*" -type f -mmin +360 -delete 2>/dev/null || true`,
+              `find ${safeShellArg(cacheParent)} -name "*.tar*" -type f -mmin +360 -delete 2>/dev/null || true`,

Similar treatment should be applied to the other find, tar, and mv invocations that include user-controlled values (including existing ones outside this diff).

Also applies to: 117-120, 125-127, 135-150, 155-157

---

129-132: Disk‑space classification based only on error message strings is brittle.

The check:

if (errorMessage.includes('No space left') || errorMessage.includes('Wrote only')) {

depends on specific tar/shell message text, which may vary by platform, locale, or implementation. This risks missing genuine ENOSPC conditions or misclassifying other errors.

Prefer also keying off structured error data where available (e.g., exit code or error.code === 'ENOSPC', or an explicit flag from CloudRunnerSystem.Run), and include other stable tokens like 'ENOSPC' in the message check.

What error properties and exit-code semantics does Node.js `child_process` expose for ENOSPC conditions from a failed `tar` command, and how stable are these across platforms?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (3)

13-19: Wrap events fetch in try-catch to prevent masking pod failures.

If listNamespacedEvent fails (e.g., RBAC restrictions, API timeout), the exception will propagate and obscure the actual pod failure. Since events are diagnostic-only, a failure here shouldn't prevent reporting the pod's failed state.

Apply this diff to wrap the events fetch:

-      const events = (await kubeClient.listNamespacedEvent(namespace)).body.items
-        .filter((x) => x.involvedObject?.name === podName)
-        .map((x) => ({
-          message: x.message || '',
-          reason: x.reason || '',
-          type: x.type || '',
-        }));
+      let events: Array<{ message: string; reason: string; type: string }> = [];
+      try {
+        events = (await kubeClient.listNamespacedEvent(namespace)).body.items
+          .filter((x) => x.involvedObject?.name === podName)
+          .map((x) => ({
+            message: x.message || '',
+            reason: x.reason || '',
+            type: x.type || '',
+          }));
+      } catch (eventError) {
+        CloudRunnerLogger.logWarning(`Failed to fetch pod events: ${eventError}`);
+      }

---

35-58: Multi-container logic flaw: only the last container's exit status is retained.

The loop iterates over all container statuses but overwrites containerExitCode and containerSucceeded on each iteration. If a pod has multiple containers and the last one succeeded but an earlier one failed, the code will incorrectly treat the pod as successful and skip error throwing at line 147.

Consider checking if all containers succeeded:

-      let containerExitCode: number | undefined;
-      let containerSucceeded = false;
+      let allContainersSucceeded = true;
+      let anyContainerExitCode: number | undefined;
+      let anyContainerFailed = false;
       
       if (containerStatuses.length > 0) {
         containerStatuses.forEach((cs, idx) => {
           if (cs.state?.waiting) {
             errorDetails.push(
               `Container ${idx} (${cs.name}) waiting: ${cs.state.waiting.reason} - ${cs.state.waiting.message || ''}`,
             );
           }
           if (cs.state?.terminated) {
             const exitCode = cs.state.terminated.exitCode;
-            containerExitCode = exitCode;
+            anyContainerExitCode = exitCode;
             if (exitCode === 0) {
-              containerSucceeded = true;
+              // Keep checking all containers
+            } else {
+              allContainersSucceeded = false;
+              anyContainerFailed = true;
             }
             errorDetails.push(
               `Container ${idx} (${cs.name}) terminated: ${cs.state.terminated.reason} - ${
                 cs.state.terminated.message || ''
               } (exit code: ${exitCode})`,
             );
           }
         });
       }

Then update line 71 to use allContainersSucceeded && !anyContainerFailed and refactor the subsequent logic accordingly.

---

89-123: Retry logic only checks the first container, inconsistent with multi-container handling.

Line 101 explicitly accesses updatedPod.status.containerStatuses[0], checking only the first container. This is inconsistent with the earlier loop (lines 39-57) that iterates over all containers, and it assumes single-container pods.

Issue: In multi-container pods, if the first container succeeds (exit code 0) but another container fails, the retry logic at line 108 will return false, incorrectly treating the pod as successful.

Consider iterating over all container statuses in the retry logic:

           const updatedPod = (
             await kubeClient.listNamespacedPod(namespace)
           ).body.items.find((x) => podName === x.metadata?.name);
           if (updatedPod?.status?.containerStatuses && updatedPod.status.containerStatuses.length > 0) {
-            const updatedContainerStatus = updatedPod.status.containerStatuses[0];
-            if (updatedContainerStatus.state?.terminated) {
-              const updatedExitCode = updatedContainerStatus.state.terminated.exitCode;
-              if (updatedExitCode === 0) {
-                CloudRunnerLogger.logWarning(
-                  `Pod ${podName} container succeeded (exit code 0) after waiting. PreStopHook failure is non-fatal.`,
-                );
-                return false; // Pod is not running, but container succeeded
-              } else {
-                CloudRunnerLogger.log(
-                  `Pod ${podName} container failed with exit code ${updatedExitCode} after waiting.`,
-                );
-                errorDetails.push(`Container terminated after wait: exit code ${updatedExitCode}`);
-                containerExitCode = updatedExitCode;
-                containerSucceeded = false;
-                break;
+            let allSucceeded = true;
+            let anyTerminated = false;
+            for (const cs of updatedPod.status.containerStatuses) {
+              if (cs.state?.terminated) {
+                anyTerminated = true;
+                const exitCode = cs.state.terminated.exitCode;
+                if (exitCode !== 0) {
+                  allSucceeded = false;
+                  errorDetails.push(
+                    `Container (${cs.name}) terminated after wait: exit code ${exitCode}`,
+                  );
+                }
               }
             }
+            if (anyTerminated) {
+              if (allSucceeded) {
+                CloudRunnerLogger.logWarning(
+                  `Pod ${podName} all containers succeeded after waiting. PreStopHook failure is non-fatal.`,
+                );
+                return false;
+              }
+              break; // At least one container failed
+            }
           }

.github/workflows/cloud-runner-integrity.yml (7)

274-292: Replace GITHUB_TOKEN override with default job token (LocalStack job: all 10 test steps).

All test steps in the localstack job override GITHUB_TOKEN with ${{ secrets.GIT_PRIVATE_TOKEN }}, violating least-privilege. Keep GIT_PRIVATE_TOKEN as a separate variable; let the code's fallback logic use it. Replace across all steps at lines 291–292, 315–316, 339–340, 363–364, 387–388, 411–412, 435–436, 459–460, 483–484, and 507–508.

Example diff for the first test step:

       - name: Run cloud-runner-end2end-locking test
         timeout-minutes: 60
         run: yarn run test "cloud-runner-end2end-locking" --detectOpenHandles --forceExit --runInBand
         env:
           UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
           UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
           UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
           PROJECT_PATH: test-project
           TARGET_PLATFORM: StandaloneWindows64
           cloudRunnerTests: true
           versioning: None
           KUBE_STORAGE_CLASS: local-path
           PROVIDER_STRATEGY: aws
           AWS_ACCESS_KEY_ID: test
           AWS_SECRET_ACCESS_KEY: test
           AWS_ENDPOINT: http://localhost:4566
           AWS_ENDPOINT_URL: http://localhost:4566
           GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
-          GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+          GITHUB_TOKEN: ${{ github.token }}

Apply the same change to all remaining test steps in the localstack job.

---

274-292: Add AWS_S3_FORCE_PATH_STYLE and AWS_EC2_METADATA_DISABLED to all LocalStack test steps.

The k8s job (lines 157–158, 197–198, 237–238) correctly includes these variables for LocalStack compatibility. All 10 test steps in the localstack job are missing them, leading to inconsistent behavior. Add to all test steps in the localstack job.

Starting with the first test step (cloud-runner-end2end-locking):

       - name: Run cloud-runner-end2end-locking test
         timeout-minutes: 60
         run: yarn run test "cloud-runner-end2end-locking" --detectOpenHandles --forceExit --runInBand
         env:
           UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
           UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
           UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
           PROJECT_PATH: test-project
           TARGET_PLATFORM: StandaloneWindows64
           cloudRunnerTests: true
           versioning: None
           KUBE_STORAGE_CLASS: local-path
           PROVIDER_STRATEGY: aws
           AWS_ACCESS_KEY_ID: test
           AWS_SECRET_ACCESS_KEY: test
           AWS_ENDPOINT: http://localhost:4566
           AWS_ENDPOINT_URL: http://localhost:4566
+          AWS_S3_FORCE_PATH_STYLE: 'true'
+          AWS_EC2_METADATA_DISABLED: 'true'
           GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
           GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}

Apply the same additions (AWS_S3_FORCE_PATH_STYLE: 'true' and AWS_EC2_METADATA_DISABLED: 'true') to the env blocks of all remaining test steps in the localstack job (lines 298–316, 322–340, 346–364, 370–388, 394–412, 418–436, 442–460, 466–484, 490–508).

---

135-160: Replace GITHUB_TOKEN override with default job token (k8s job: first test).

Setting GITHUB_TOKEN to ${{ secrets.GIT_PRIVATE_TOKEN }} overrides the runner-provided token and exposes a broader-scope PAT. Keep GIT_PRIVATE_TOKEN separate if private-repo access is needed; let the code's fallback logic handle it.

Apply this diff:

       - name: Run cloud-runner-end2end-caching test
         timeout-minutes: 60
         run: yarn run test "cloud-runner-end2end-caching" --detectOpenHandles --forceExit --runInBand
         env:
           UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
           UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
           UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
           PROJECT_PATH: test-project
           TARGET_PLATFORM: StandaloneWindows64
           cloudRunnerTests: true
           versioning: None
           KUBE_STORAGE_CLASS: local-path
           PROVIDER_STRATEGY: k8s
           containerCpu: '512'
           containerMemory: '512'
           AWS_ACCESS_KEY_ID: test
           AWS_SECRET_ACCESS_KEY: test
           AWS_S3_ENDPOINT: http://localhost:4566
           AWS_ENDPOINT: http://localhost:4566
           INPUT_AWSS3ENDPOINT: http://localhost:4566
           INPUT_AWSENDPOINT: http://localhost:4566
           AWS_S3_FORCE_PATH_STYLE: 'true'
           AWS_EC2_METADATA_DISABLED: 'true'
           GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
-          GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+          GITHUB_TOKEN: ${{ github.token }}

---

176-200: Replace GITHUB_TOKEN override in retaining test (k8s job).

Same token concern as caching test. Use the default job token instead.

Apply this diff:

       - name: Run cloud-runner-end2end-retaining test
         timeout-minutes: 60
         run: yarn run test "cloud-runner-end2end-retaining" --detectOpenHandles --forceExit --runInBand
         env:
           UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
           UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
           UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
           PROJECT_PATH: test-project
           TARGET_PLATFORM: StandaloneWindows64
           cloudRunnerTests: true
           versioning: None
           KUBE_STORAGE_CLASS: local-path
           PROVIDER_STRATEGY: k8s
           containerCpu: '512'
           containerMemory: '512'
           AWS_ACCESS_KEY_ID: test
           AWS_SECRET_ACCESS_KEY: test
           AWS_S3_ENDPOINT: http://localhost:4566
           AWS_ENDPOINT: http://localhost:4566
           INPUT_AWSS3ENDPOINT: http://localhost:4566
           INPUT_AWSENDPOINT: http://localhost:4566
           AWS_S3_FORCE_PATH_STYLE: 'true'
           AWS_EC2_METADATA_DISABLED: 'true'
           GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
-          GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+          GITHUB_TOKEN: ${{ github.token }}

---

216-240: Replace GITHUB_TOKEN override in hooks test (k8s job).

Same token concern. Use the default job token instead.

Apply this diff:

       - name: Run cloud-runner-hooks test
         timeout-minutes: 60
         run: yarn run test "cloud-runner-hooks" --detectOpenHandles --forceExit --runInBand
         env:
           UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
           UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
           UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
           PROJECT_PATH: test-project
           TARGET_PLATFORM: StandaloneWindows64
           cloudRunnerTests: true
           versioning: None
           KUBE_STORAGE_CLASS: local-path
           PROVIDER_STRATEGY: k8s
           containerCpu: '512'
           containerMemory: '512'
           AWS_ACCESS_KEY_ID: test
           AWS_SECRET_ACCESS_KEY: test
           AWS_S3_ENDPOINT: http://localhost:4566
           AWS_ENDPOINT: http://localhost:4566
           INPUT_AWSS3ENDPOINT: http://localhost:4566
           INPUT_AWSENDPOINT: http://localhost:4566
           AWS_S3_FORCE_PATH_STYLE: 'true'
           AWS_EC2_METADATA_DISABLED: 'true'
           GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
-          GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+          GITHUB_TOKEN: ${{ github.token }}

---

70-73: Add region-aware S3 bucket creation for LocalStack.

S3 requires LocationConstraint outside us-east-1. The current code doesn't account for this and will silently fail.

Apply this diff:

       - name: Create S3 bucket for tests (host LocalStack)
         run: |
-          awslocal s3 mb s3://$AWS_STACK_NAME || aws --endpoint-url=http://localhost:4566 s3 mb s3://$AWS_STACK_NAME || true
+          if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || aws --endpoint-url=http://localhost:4566 s3api create-bucket --bucket "$AWS_STACK_NAME" || true
+          else
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" --create-bucket-configuration LocationConstraint="${AWS_REGION}" || aws --endpoint-url=http://localhost:4566 s3api create-bucket --bucket "$AWS_STACK_NAME" --create-bucket-configuration LocationConstraint="${AWS_REGION}" || true
+          fi
           awslocal s3 ls || aws --endpoint-url=http://localhost:4566 s3 ls || echo "S3 bucket listing completed"

---

40-43: Pin k3d version and avoid curl | bash installation.

Line 42 uses an unpinned install script, which poses a supply-chain risk. Replace with a pinned binary download:

      - name: Install k3d
        run: |
-         curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash
+         set -euo pipefail
+         K3D_VERSION=v5.8.3
+         curl -fsSL -o /usr/local/bin/k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64"
+         chmod +x /usr/local/bin/k3d
          k3d version | cat

Consider storing K3D_VERSION in the workflow env section for easier updates.

src/model/cloud-runner/remote-client/caching.ts (1)

96-188: Command injection risk, over‑broad cleanup scope, fragile ENOSPC detection, and dead retrySucceeded logic.

Several concerns in this block:

1. Command injection & unsafe interpolation

   • Interpolated values like cacheFolder, cacheArtifactName, compressionSuffix, cacheParent, and cacheRoot are inserted directly into shell strings for find, rm, tar, mkdir -p, etc. (e.g., Lines 100–107, 119, 125–127, 139–142, 147–149).
   • They ultimately come from CLI/options/environment and can contain spaces or shell metacharacters, making this a command‑injection risk. The current .replace(' ', '') only strips the first space and doesn’t prevent other dangerous characters.

   Suggestion: Refactor this logic to avoid building raw shell command strings:

   • Prefer Node’s fs.promises and path for deletes and directory cleanup instead of find/rm.
   • If you absolutely must call shell utilities, expose a CloudRunnerSystem.runCommand(cmd: string, args: string[]) (or similar) that uses spawn/execFile without going through a shell, and validate all user‑controlled inputs with a strict whitelist (e.g. /^[A-Za-z0-9._-]+$/ for artifact names and a vetted set of cache base directories).
   • At minimum, validate cacheFolder/cacheParent/cacheRoot against an allow‑listed base path before using them in destructive commands.

2. Over‑broad cleanup scope → potential data loss

   • Cleanup uses cacheParent = path.dirname(cacheFolder) and cacheRoot = path.resolve(cacheParent, '..'), then runs find ${cacheParent} ... -delete and find ${cacheRoot} ... -delete on "*.tar*" (Lines 100–107, 139–142, 144–149).
   • If cacheFolder is relative (e.g., 'cache'), cacheParent may be '.' and cacheRoot may be the project root, so you may end up deleting any .tar* file in the working directory and its parent, not just cache artifacts.
   • This is the same “overly broad cleanup scope” concern raised in the previous review and remains unresolved.

   Suggestion: Restrict deletion strictly to the configured cache directory tree and the expected filename pattern:

   • Use cacheFolder (not its parent) as the search root.
   • Match a more specific pattern like "${cacheArtifactName}.tar${compressionSuffix}" or a well‑defined prefix (e.g., unity-cache-*.tar${compressionSuffix}), not all *.tar*.
   • Consider a configurable retention policy with a clear cap on the number/age of cache entries.

3. Fragile disk‑space error detection

   • The retry path checks only errorMessage.includes('No space left') || errorMessage.includes('Wrote only') (Line 131). Different platforms/tar implementations may use different messages, so real ENOSPC errors could slip through or be misclassified.

   Suggestion: Make the detection more robust by also checking standard error codes/errno:

   • Include checks like error?.code === 'ENOSPC' or errorMessage.includes('ENOSPC') and (if available) process exit codes, in addition to the current string patterns.

4. Dead / unnecessary retrySucceeded flag

   • retrySucceeded is initialized to false, set to true if the retry succeeds, and the retry’s catch unconditionally throws (Lines 153–166).
   • This means the if (!retrySucceeded) { throw error; } branch (Lines 168–170) is effectively unreachable: either the retry succeeds (flag true, condition false) or the retry throws (you never reach the if).

   You can simplify this block:

-              // Retry the tar operation once after cleanup
-              let retrySucceeded = false;
-              try {
-                await CloudRunnerSystem.Run(
-                  `tar -cf ${cacheArtifactName}.tar${compressionSuffix} "${path.basename(sourceFolder)}"`,
-                );
-                // If retry succeeds, mark it - we'll continue normally without throwing
-                retrySucceeded = true;
-              } catch (retryError: any) {
-                throw new Error(
-                  `Failed to create cache archive after cleanup. Original error: ${errorMessage}. Retry error: ${
-                    retryError?.message || retryError
-                  }`,
-                );
-              }
-              // If retry succeeded, don't throw the original error - let execution continue after catch block
-              if (!retrySucceeded) {
-                throw error;
-              }
-              // If we get here, retry succeeded - execution will continue after the catch block
+              // Retry the tar operation once after cleanup
+              try {
+                await CloudRunnerSystem.Run(
+                  `tar -cf ${cacheArtifactName}.tar${compressionSuffix} "${path.basename(sourceFolder)}"`,
+                );
+                // If we get here, retry succeeded and execution will continue after the catch block
+              } catch (retryError: any) {
+                throw new Error(
+                  `Failed to create cache archive after cleanup. Original error: ${errorMessage}. Retry error: ${
+                    retryError?.message || retryError
+                  }`,
+                );
+              }

These issues were previously flagged in earlier review comments and still apply to the updated implementation.

src/model/cloud-runner/remote-client/caching.ts (1)

82-95: Disk-usage parsing is fragile but acceptable; consider tightening.

df . 2>/dev/null || df /data ... plus /(\d+)%/ works in many environments but is somewhat brittle (locale/format differences, multiple filesystems). Optional improvement: use df -P for a more stable parse format and explicitly pick the last line’s %use (e.g., split by newline, then whitespace) instead of the first regex match.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

.github/workflows/cloud-runner-integrity.yml (2)

40-43: Unsafe k3d installation pattern remains; pin version instead.

The workflow still uses curl | bash to install k3d, which has supply-chain risk and poor reproducibility. A previous review flagged this (marked as "Addressed in commit 69731ba") but the unsafe pattern persists.

Replace with a pinned release download:

-      - name: Install k3d
+      - name: Install k3d (pinned)
         run: |
+          set -euo pipefail
+          K3D_VERSION=v5.7.3
-          curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash
+          mkdir -p /tmp/k3d && cd /tmp/k3d
+          curl -fsSL -o k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64"
+          chmod +x k3d
+          sudo mv k3d /usr/local/bin/k3d
           k3d version | cat

---

164-165: Stop overriding GITHUB_TOKEN with GIT_PRIVATE_TOKEN across all test steps.

The workflow sets GITHUB_TOKEN to ${{ secrets.GIT_PRIVATE_TOKEN }} in 13 test steps across both jobs (k8s: 3 steps; localstack: 10 steps). This violates least-privilege by unnecessarily exposing broader-scope PATs and overrides GitHub's automatically-provided workflow token.

Replace all occurrences with:

- GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+ GITHUB_TOKEN: ${{ github.token }}

Keep GIT_PRIVATE_TOKEN as a separate variable if private repo access is truly required.

Locations: 164-165, 204-205, 244-245, 296-297, 320-321, 344-345, 368-369, 392-393, 416-417, 440-441, 464-465, 488-489, 512-513

.github/workflows/cloud-runner-integrity.yml (2)

73-76: S3 bucket creation should use region-aware API for robustness.

The current approach uses awslocal s3 mb with an AWS CLI fallback, which doesn't handle region-aware bucket creation. For regions other than us-east-1, S3 requires a LocationConstraint in the CreateBucket request.

Replace with region-aware logic:

-          awslocal s3 mb s3://$AWS_STACK_NAME || aws --endpoint-url=http://localhost:4566 s3 mb s3://$AWS_STACK_NAME || true
-          awslocal s3 ls || aws --endpoint-url=http://localhost:4566 s3 ls || echo "S3 bucket listing completed"
+          # Use s3api with region-aware LocationConstraint
+          REGION=${AWS_REGION:-us-east-1}
+          if [ "$REGION" = "us-east-1" ]; then
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" 2>/dev/null || true
+          else
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \
+              --create-bucket-configuration LocationConstraint="$REGION" 2>/dev/null || true
+          fi
+          awslocal s3 ls || echo "S3 bucket listing completed"

---

118-139: Consider extracting repeated K8s cleanup logic into a reusable step or composite action.

The K8s job repeats the cleanup logic (delete jobs, pods, PVCs, secrets, cache, prune Docker) three times verbatim. This creates maintenance burden—changes must be made in three places.

Consider extracting to a composite action or parameterized step for maintainability:

- name: Clean up K8s resources and disk
  uses: ./.github/actions/cleanup-k8s-test-resources
  with:
    namespace: default

For now, this is a minor maintainability observation; functionality is correct.

Also applies to: 166-180, 206-220

src/model/cloud-runner/providers/k8s/kubernetes-job-spec-factory.ts (2)

66-66: Termination grace vs preStop hook look consistent; preStop command is dense but OK

Setting terminationGracePeriodSeconds: 90 matches the preStop hook doing a sleep 60 plus a short license‑return script, so the pod should have enough time for the hook to complete before Kubernetes force‑kills it. That alignment looks good.

The preStop shell command itself is quite compact and uses &&/|| chaining, which is correct but a bit hard to read/maintain. If you touch this again, consider extracting the full command string into a named constant or slightly simplifying it (e.g., dropping the cd if /steps/return_license.sh is always absolute), but there’s no immediate functional issue here.

Also applies to: 120-130

---

25-49: Reconsider hard-coded k3d endpoint rewriting without cluster detection

The AWS endpoint rewriting for k3d is sensible for the local dev case, but the implementation has one concern:

• The host.k3d.internal mapping is applied unconditionally to AWS endpoint vars whenever localhost URLs are detected. However, line 138 shows that the code can detect CLOUD_RUNNER_MINIKUBE environments. Other clusters (minikube, kind, EKS) use different gateway hostnames or may not need this rewrite at all. Consider either:
  • Adding a similar detection flag for k3d and gating the rewrite logic
  • Making the gateway hostname configurable (e.g., CLOUD_RUNNER_HOST_GATEWAY env var)

The object literal mapping on line 48 is fine as-is; CloudRunnerEnvironmentVariable has only name and value properties, so no fields are being dropped.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (3)

13-19: Wrap events fetch in try-catch to prevent masking pod failures.

If listNamespacedEvent fails (e.g., RBAC restrictions, API timeout), the exception will propagate and obscure the actual pod failure. Since events are diagnostic-only, a failure here shouldn't prevent reporting the pod's failed state.

-      const events = (await kubeClient.listNamespacedEvent(namespace)).body.items
-        .filter((x) => x.involvedObject?.name === podName)
-        .map((x) => ({
-          message: x.message || '',
-          reason: x.reason || '',
-          type: x.type || '',
-        }));
+      let events: Array<{ message: string; reason: string; type: string }> = [];
+      try {
+        events = (await kubeClient.listNamespacedEvent(namespace)).body.items
+          .filter((x) => x.involvedObject?.name === podName)
+          .map((x) => ({
+            message: x.message || '',
+            reason: x.reason || '',
+            type: x.type || '',
+          }));
+      } catch (eventError) {
+        CloudRunnerLogger.logWarning(`Failed to fetch pod events: ${eventError}`);
+      }

---

35-58: Multi-container logic flaw: only the last container's exit status is retained.

The loop iterates over all container statuses but overwrites containerExitCode and containerSucceeded on each iteration (lines 47-49). If a pod has multiple containers and the last one succeeded but an earlier one failed, the code will incorrectly treat the pod as successful and skip error throwing at line 159.

Consider checking if all containers succeeded:

-      let containerExitCode: number | undefined;
-      let containerSucceeded = false;
+      let allContainersSucceeded = true;
+      let anyContainerExitCode: number | undefined;
       
       if (containerStatuses.length > 0) {
         containerStatuses.forEach((cs, idx) => {
           if (cs.state?.waiting) {
             errorDetails.push(
               `Container ${idx} (${cs.name}) waiting: ${cs.state.waiting.reason} - ${cs.state.waiting.message || ''}`,
             );
           }
           if (cs.state?.terminated) {
             const exitCode = cs.state.terminated.exitCode;
-            containerExitCode = exitCode;
+            anyContainerExitCode = exitCode;
             if (exitCode === 0) {
-              containerSucceeded = true;
+              // Keep checking all containers
+            } else {
+              allContainersSucceeded = false;
             }
             errorDetails.push(
               `Container ${idx} (${cs.name}) terminated: ${cs.state.terminated.reason} - ${
                 cs.state.terminated.message || ''
               } (exit code: ${exitCode})`,
             );
           }
         });
       }

Then update the logic at lines 71, 89, 126, 138, and 148 to use allContainersSucceeded and anyContainerExitCode.

---

89-123: Retry logic only checks the first container, inconsistent with multi-container handling.

Line 101 explicitly accesses updatedPod.status.containerStatuses[0], checking only the first container. This is inconsistent with the earlier loop (lines 39-57) that iterates over all containers, and assumes single-container pods.

Issue: In multi-container pods, if the first container succeeds (exit code 0) but another container fails, the retry logic at line 108 will return false, incorrectly treating the pod as successful.

Consider iterating over all container statuses in the retry logic:

           const updatedPod = (
             await kubeClient.listNamespacedPod(namespace)
           ).body.items.find((x) => podName === x.metadata?.name);
           if (updatedPod?.status?.containerStatuses && updatedPod.status.containerStatuses.length > 0) {
-            const updatedContainerStatus = updatedPod.status.containerStatuses[0];
-            if (updatedContainerStatus.state?.terminated) {
-              const updatedExitCode = updatedContainerStatus.state.terminated.exitCode;
-              if (updatedExitCode === 0) {
-                CloudRunnerLogger.logWarning(
-                  `Pod ${podName} container succeeded (exit code 0) after waiting. PreStopHook failure is non-fatal.`,
-                );
-                return false; // Pod is not running, but container succeeded
-              } else {
-                CloudRunnerLogger.log(
-                  `Pod ${podName} container failed with exit code ${updatedExitCode} after waiting.`,
-                );
-                errorDetails.push(`Container terminated after wait: exit code ${updatedExitCode}`);
-                containerExitCode = updatedExitCode;
-                containerSucceeded = false;
-                break;
+            let allSucceeded = true;
+            let anyTerminated = false;
+            for (const cs of updatedPod.status.containerStatuses) {
+              if (cs.state?.terminated) {
+                anyTerminated = true;
+                const exitCode = cs.state.terminated.exitCode;
+                if (exitCode !== 0) {
+                  allSucceeded = false;
+                  errorDetails.push(
+                    `Container (${cs.name}) terminated after wait: exit code ${exitCode}`,
+                  );
+                }
               }
             }
+            if (anyTerminated) {
+              if (allSucceeded) {
+                CloudRunnerLogger.logWarning(
+                  `Pod ${podName} all containers succeeded after waiting. PreStopHook failure is non-fatal.`,
+                );
+                return false;
+              }
+              break; // At least one container failed
+            }
           }

.github/workflows/cloud-runner-integrity.yml (3)

48-51: Use region-aware S3 API to support non-us-east-1 regions.

S3 requires LocationConstraint outside us-east-1. The current awslocal s3 mb command will fail for non-us-east-1 regions. This was flagged in prior feedback as accepted but not yet applied.

Apply this diff to support all AWS regions:

       - name: Create S3 bucket for tests (host LocalStack)
         run: |
+          if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true
+          else
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \
+              --create-bucket-configuration LocationConstraint="${AWS_REGION}" || true
+          fi
-          awslocal s3 mb s3://$AWS_STACK_NAME || true
           awslocal s3 ls

---

252-267: Add AWS_S3_FORCE_PATH_STYLE and AWS_EC2_METADATA_DISABLED to all 10 LocalStack test steps.

The k8s job correctly includes AWS_S3_FORCE_PATH_STYLE: 'true' and AWS_EC2_METADATA_DISABLED: 'true', but all 10 test steps in the localstack job omit these required environment variables. This creates inconsistent LocalStack behavior between jobs.

Add these variables to every test step's env block in the localstack job:

         PROVIDER_STRATEGY: aws
         AWS_ACCESS_KEY_ID: test
         AWS_SECRET_ACCESS_KEY: test
         AWS_ENDPOINT: http://localhost:4566
         AWS_ENDPOINT_URL: http://localhost:4566
+        AWS_S3_FORCE_PATH_STYLE: 'true'
+        AWS_EC2_METADATA_DISABLED: 'true'
         GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
         GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}

Apply to: cloud-runner-end2end-locking, cloud-runner-end2end-caching, cloud-runner-end2end-retaining, cloud-runner-caching, cloud-runner-environment, cloud-runner-image, cloud-runner-hooks, cloud-runner-local-persistence, cloud-runner-locking-core, and cloud-runner-locking-get-locked tests.

---

40-43: Pin k3d version and avoid curl | bash to reduce supply-chain risk.

The workflow currently downloads and executes an unpinned install script from the main branch, which is unverifiable and poses a supply-chain risk. The official k3d documentation recommends downloading the binary directly instead.

Apply this diff to pin the version and download the binary with checksum verification:

-      - name: Install k3d
+      - name: Install k3d (pinned)
         run: |
+          set -euo pipefail
+          K3D_VERSION=v5.7.3
+          curl -fsSL -o /usr/local/bin/k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64"
+          chmod +x /usr/local/bin/k3d
-          curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash
           k3d version | cat

.github/workflows/cloud-runner-integrity.yml (1)

88-110: Extract repetitive K8s cleanup logic into a reusable step or helper.

The K8s cleanup logic is duplicated verbatim across three cleanup steps (lines 88–110, 136–150, 176–190). This violates DRY and makes future maintenance harder.

Consider extracting this into a composite action or a separate shell script that can be sourced. Example using a composite action:

- name: Clean up K8s test resources
  uses: ./.github/actions/k8s-cleanup

Then create .github/actions/k8s-cleanup/action.yml with the shared cleanup logic.

Also applies to: 136-150, 176-190

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

.github/workflows/cloud-runner-integrity.yml (4)

40-43: Replace unsafe curl | bash with pinned k3d release.

The workflow still uses an unpinned curl | bash invocation, which poses supply-chain risk and reproducibility concerns. This was flagged in previous reviews.

Apply this diff to download a specific k3d release:

-      - name: Install k3d
+      - name: Install k3d (pinned)
         if: ${{ matrix.provider == 'k8s' }}
         run: |
-          curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash
+          set -euo pipefail
+          K3D_VERSION=v5.7.3
+          curl -fsSL -o /usr/local/bin/k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64"
+          chmod +x /usr/local/bin/k3d
           k3d version | cat

---

48-51: Use region-aware S3 bucket creation for LocalStack.

The workflow creates the S3 bucket without specifying a region, which fails for non-us-east-1 deployments. S3 requires LocationConstraint outside us-east-1. This was flagged in previous reviews.

Apply this diff to handle region-aware bucket creation:

       - name: Create S3 bucket for tests (host LocalStack)
         run: |
-          awslocal s3 mb s3://$AWS_STACK_NAME || true
+          if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true
+          else
+            awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \
+              --create-bucket-configuration LocationConstraint="${AWS_REGION}" || true
+          fi
           awslocal s3 ls

Ensure AWS_REGION is set in the env block (currently lines 17–25), or default to us-east-1 if not set.

---

147-147: Stop overriding GITHUB_TOKEN with GIT_PRIVATE_TOKEN across 13 test steps.

The workflow sets GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} in all test steps, which:

• Exposes a broader-scope PAT unnecessarily to each job
• Breaks the intended fallback pattern in the code (try GIT_PRIVATE_TOKEN first, then fall back to GITHUB_TOKEN)
• Violates the principle of least privilege

This was flagged in previous reviews and remains unaddressed.

Replace the pattern across all test steps to use the default runner-provided token:

  GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
- GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+ GITHUB_TOKEN: ${{ github.token }}

Keep GIT_PRIVATE_TOKEN as a separate variable if private repo access is required; the code has proper fallback logic to use it.

Also applies to: 187-187, 227-227, 279-279, 303-303, 327-327, 351-351, 375-375, 399-399, 423-423, 447-447, 471-471, 495-495

---

276-279: Add AWS_S3_FORCE_PATH_STYLE and AWS_EC2_METADATA_DISABLED to all LocalStack test steps.

The k8s job correctly includes AWS_S3_FORCE_PATH_STYLE: 'true' and AWS_EC2_METADATA_DISABLED: 'true' (lines 144–145, 184–185, 224–225) for LocalStack compatibility, but the localstack job omits these across all 10 test steps. This creates inconsistent behavior and was flagged in previous reviews.

Add these variables to every test step in the localstack job. For example, lines 276–279 should become:

         AWS_ENDPOINT: http://localhost:4566
         AWS_ENDPOINT_URL: http://localhost:4566
+        AWS_S3_FORCE_PATH_STYLE: 'true'
+        AWS_EC2_METADATA_DISABLED: 'true'
         GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}

Apply the same additions to all other test steps in the localstack job (lines 300–303, 324–327, 348–351, 372–375, 396–399, 420–423, 444–447, 468–471, 492–495).

Also applies to: 300-303, 324-327, 348-351, 372-375, 396-399, 420-423, 444-447, 468-471, 492-495

.github/workflows/cloud-runner-integrity.yml (1)

74-81: Remove duplicate cleanup code in k8s job.

Lines 74–81 duplicate the cleanup logic from lines 74–81 (disk cleanup and docker system prune). Consolidate or remove the redundant block to improve readability.

-          # Clean up disk space on the k3d node to prevent evictions
-          echo "Cleaning up disk space on k3d nodes..."
-          docker exec k3d-unity-builder-agent-0 sh -c "df -h && docker system prune -af --volumes || true" || true
-          docker system prune -af --volumes || true
-          # Clean up disk space on the node to prevent evictions
-          echo "Cleaning up disk space on k3d nodes..."
-          docker exec k3d-unity-builder-agent-0 sh -c "df -h && docker system prune -af --volumes || true" || true
-          docker system prune -af --volumes || true

src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (2)

56-65: Consider clarifying the fallback logic and using a constant for error matching.

The fallback recovery path is well-designed, but the condition at line 57 is complex. Consider:

1. Adding a comment explaining that both isRunning (pre-error) and continueStreaming (post-error) are checked to ensure the pod wasn't running before and after the error occurred.
2. Extracting the error message string 'previous terminated container' to a named constant to improve maintainability and make the error detection more explicit.

Example refactor:

+  // kubectl error message when previous container logs are unavailable
+  static readonly PREVIOUS_CONTAINER_NOT_FOUND = 'previous terminated container';
+
   static async runTask(
     ...
   ) {
     ...
       } catch (error: any) {
         await new Promise((resolve) => setTimeout(resolve, 3000));
         const continueStreaming = await KubernetesPods.IsPodRunning(podName, namespace, kubeClient);
         CloudRunnerLogger.log(`K8s logging error ${error} ${continueStreaming}`);
-        // If pod is not running and we tried --previous but it failed, try without --previous
-        if (!isRunning && !continueStreaming && error?.message?.includes('previous terminated container')) {
+        // If pod wasn't running initially and still isn't running after error, 
+        // and we got a "previous container not found" error, try fetching current container logs
+        if (!isRunning && !continueStreaming && error?.message?.includes(KubernetesTaskRunner.PREVIOUS_CONTAINER_NOT_FOUND)) {

---

74-78: Verify non-fatal handling is appropriate for all scenarios.

Treating the "previous terminated container" error as non-fatal improves resilience, but ensure this behavior is acceptable for all use cases where previous container logs might be critical for debugging.

Also consider using the same constant suggested in the previous comment for consistency.

-        if (error?.message?.includes('previous terminated container')) {
+        if (error?.message?.includes(KubernetesTaskRunner.PREVIOUS_CONTAINER_NOT_FOUND)) {
           CloudRunnerLogger.logWarning(`Could not fetch previous container logs, but continuing...`);
           break;
         }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (1)

55-90: Fallback and retry logic looks robust; consider clarifying retry counter semantics

The new fallback when previous‑container logs are unavailable, plus the special‑case handling of "previous terminated container" errors, makes the log streaming much more resilient and avoids failing builds just because prior container logs are missing.

At the same time, retriesAfterFinish is now used as a general retry counter for several cases (fallback attempts, generic log errors, and “no end marker” scenarios), not only “after finish”. The name is a bit misleading and makes it harder to reason about when the loop will actually stop.

To improve readability and future maintenance, consider:

• Renaming retriesAfterFinish to something more generic like logRetryCount or logErrorRetries.
• Adding a short comment near its declaration documenting that it governs both fallback and general log‑error retries, and that exceeding maxRetry for "previous terminated container" errors is treated as non‑fatal, while other errors are re‑thrown.

This doesn’t change behavior but makes the control flow easier to understand for future contributors.

Also applies to: 100-107

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/model/cloud-runner/remote-client/index.ts (2)

282-282: Fix contradictory LFS folder assertion as previously flagged.

The assertion checks that the LFS folder exists (fs.existsSync() returns true), but the message states it "should not exist before caching." This contradiction was flagged in a previous review.

Either invert the check to !fs.existsSync(path.join('.git', 'lfs')) or update the message to match the actual assertion.

---

242-268: Fix unused catch bindings as previously flagged.

This issue was already identified in a previous review. Remove unused catch bindings (_error, _error2, _error3) and use parameterless catch blocks where the error is not used, or name the caught error appropriately when it is thrown.

Based on past review comments, the recommended fix is to use catch without parameters for unused errors, and preserve the error variable where it's re-thrown.

src/model/cloud-runner/workflows/build-automation-workflow.ts (1)

201-206: Non-containerized providers do not execute the build.

The non-containerized code path never copies the build scripts or executes /entrypoint.sh, so no actual Unity build occurs. The workflow only logs markers and runs post-build tasks against non-existent build outputs.

This confirms the issue flagged in the previous review: "Local provider no longer executes build entrypoint... local runs will immediately log success without invoking any build logic, so local provider builds will silently do nothing."

The non-containerized path should stage and execute the build scripts similar to the containerized paths. Apply this structure:

 return `
     echo "game ci start"
     echo "game ci start" >> "$LOG_FILE"
+    mkdir -p ${`${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.projectBuildFolderAbsolute)}/build`}
+    cd ${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.projectPathAbsolute)}
+    cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(distFolder, 'default-build-script'))}" "/UnityBuilderAction"
+    cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'entrypoint.sh'))}" "/entrypoint.sh"
+    cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'steps'))}" "/steps"
+    chmod -R +x "/entrypoint.sh"
+    chmod -R +x "/steps"
+    /entrypoint.sh 2>&1 | tee -a "$LOG_FILE" &
     timeout 3s node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || true
     node ${builderPath} -m remote-cli-post-build`;

src/model/cloud-runner/remote-client/index.ts (1)

128-134: Consider consolidating logging calls.

The success message is logged three separate ways (CloudRunnerLogger, stdout, console). If CloudRunnerLogger already outputs to stdout/console, this may be redundant.

src/model/cloud-runner/workflows/build-automation-workflow.ts (1)

95-106: Consider preserving git error output for troubleshooting.

The multi-tier fallback strategy (checking remote branch, then falling back to known branches) is well-designed. However, redirecting stderr to /dev/null on line 98 may hide authentication failures or network issues that would help diagnose clone problems.

Consider logging git errors while still allowing the fallback chain to proceed:

-if [ -n "$(git ls-remote --heads \"$REPO\" \"$BRANCH\" 2>/dev/null)" ]; then
+if git ls-remote --heads "$REPO" "$BRANCH" 2>&1 | grep -q "refs/heads/$BRANCH"; then
   git clone -q -b "$BRANCH" "$REPO" "$DEST"
 else
   echo "Remote branch $BRANCH not found in $REPO; falling back to a known branch"
-  git clone -q -b cloud-runner-develop "$REPO" "$DEST" \
-    || git clone -q -b main "$REPO" "$DEST" \
-    || git clone -q "$REPO" "$DEST"
+  git clone -q -b cloud-runner-develop "$REPO" "$DEST" 2>&1 \
+    || git clone -q -b main "$REPO" "$DEST" 2>&1 \
+    || git clone -q "$REPO" "$DEST" 2>&1 \
+    || { echo "ERROR: All git clone attempts failed for $REPO"; exit 1; }
 fi

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.